Switch from ESTD to ETL #58

rolandreichweinbmw · 2025-02-07T11:36:30Z

As being discussed in #51 , this is a first request for review via pull request.

marcmo · 2025-02-08T16:52:28Z

thanks for your PR. It's impressive...it's massive! there is a lot of code that still needs formatting and it does not yet compile—those 2 things we need to solve first.

rolandreichweinbmw · 2025-02-08T20:13:39Z

Regarding code formatting: I did as "treefmt" suggested.

rolandreichweinbmw · 2025-02-08T20:20:01Z

Regarding compile: For me, it compiles and runs (on C++14).

Update: I also fixed it for the CI builds C++17 and C++20.

Or do you mean a different compile error I'm not aware of?

I compiled:

Unit tests
POSIX
S32K148

rolandreichweinbmw · 2025-02-09T20:58:29Z

BTW: I'm using my fork/branch of ETL at https://github.com/rolandreichweinbmw/etl/tree/openbsw-replace-estd - just for your reference.

It includes some additions that I already contributed upstream, but no ETL release happened yet.

christian-schilling

I'm raising a few points where I see etl being a step back from estl to put them up for discussion.
Apart from those we should think about how to treat etl and it's development. Do we want to upstream changes? Maintain patches? Keep it as is? Maintain an "add-on" library for missing pieces (like the ones I commented on).

Also I'm wondering if we really gain a lot by adopting etl. Just because we can does not mean we should.

christian-schilling · 2025-02-10T13:00:18Z

executables/referenceApp/application/src/systems/DoCanSystem.cpp

        ::util::logger::DOCAN);
 }

 void DoCanSystem::init()
 {
-    _classicAddressingFilter.init(::estd::make_slice(_addresses), ::estd::make_slice(_codecs));
+    _classicAddressingFilter.init(


Are there no comparable make_span helpers in etl that offer type deduction? I think it's quite beneficial to have/use those.

I agree that they are really helpful.

There are deduction guides to the etl::span ctors:

https://github.com/ETLCPP/etl/blob/99d75375060b61aae49bb2fc06349b45cb2c3380/include/etl/span.h#L750

Unfortunately, only available w/ ETL_USING_CPP17

Maybe we can consider switching to C++17 in OpenBSW soon? :-)

For C++14, I can provide etl::make_span helper as we know from estd.

Update:
See: https://github.com/rolandreichweinbmw/etl/tree/span-make-span
And: ETLCPP/etl#1027

Now using it accordingly here.

Agreed not sure why it was not added to etl, because they have make_array for example. Could be a suggestion for ETL

christian-schilling · 2025-02-10T13:03:16Z

executables/unitTest/common/include/util/EcuIdList.h

@@ -52,7 +53,9 @@ class EcuIdList

    uint16_t getSize() const
    {
-        return (fBufferLength == 0U) ? 0U : ::estd::read_be<uint16_t>(fpData);
+        return (fBufferLength == 0U)


This is quite a step back in readability. I wonder if this can be expressed more concisely with etl.
One of the things we tried to achieve with estl was to eleminate the need to use reinterpret_cast from most code as far as possible by providing safer alternatives. One reasoning for this is also that reinterpret_cast often triggers warnings with safty related rulesets and using a safe wrapper api seems like a better approach over always adding suppression comments for those warnings.

Agreed. I think the goal of ETL is also to achieve that "less usage of reinterpret_cast". Probably there is some other workaround for this specific use-case. Didn't try that out myself though.

I agree!

That's why I contributed two alternative solutions to ETL some time ago:

unaligned_type_ext: https://github.com/rolandreichweinbmw/etl/tree/unaligned-type-ext
separate type; read/write e.g. via etl::be_uint16_t_ext{data}

unaligned_type::at_address: https://github.com/rolandreichweinbmw/etl/tree/unaligned-type-at-address
pseudo constructor helper; e.g. etl::be_uint32_t::at_address(data)

I haven't used it yet in the above OpenBSW line since upstream has not decided yet which one to merge.

christian-schilling · 2025-02-10T13:12:46Z

libs/bsw/asyncFreeRtos/include/async/TaskInitializer.h

@@ -248,8 +258,9 @@ void TaskInitializer<Adapter>::create(
    TaskFunctionType const taskFunction,
    TaskConfigType const& config)
 {
-    ::estd::slice<uint8_t> bytes = ::estd::make_slice(stack).template reinterpret_as<uint8_t>();
-    ::estd::memory::align(alignof(StackType_t), bytes);
+    ::etl::span<uint8_t> bytes = ::etl::span<uint8_t>(


Another case of reinterpret_cast being reintroduced in place of an api specifially designed to avoid it. (reinterpret_as, which is safer as it also includes the length adjustment calculations)

Agreed! But I wonder if this could be done in another way. Or if span could be construct in a different manner, because etl also offers reinterpret_as in etl/span.h. Haven't had the time to try that out myself.

I agree. I already contributed span::reinterpret_as() some time ago at:

https://github.com/rolandreichweinbmw/etl/tree/span-advance-copy-reinterpret-as

(Not yet available in upstream release, but we can already use it if you like as it is available in the ETL copy in this PR.)

christian-schilling · 2025-02-10T13:17:39Z

libs/bsw/docan/include/docan/can/DoCanPhysicalCanTransceiver.h

 {
    (void)lastFrameIndex;
    if (!_sendPending)
    {
-        ::estd::slice<uint8_t> payload = ::estd::slice<uint8_t>::from_pointer(
+        ::etl::span<uint8_t> payload(


We chose to use from_pointer intead of overloading the constructor to make this less-safe and therefore less preferable way of creating slices/spans easier to spot.

Makes sense. Could be something to suggest also for ETL

christian-schilling · 2025-02-10T13:26:20Z

libs/bsw/docan/test/gtest/src/docan/addressing/DoCanNormalAddressingTest.cpp

-    ::estd::slice<uint8_t> payload  = payloadBuffer;
-    EXPECT_TRUE(::estd::memory::is_equal(expectedPayload, payload));
+    ::etl::span<uint8_t> payload    = payloadBuffer;
+    EXPECT_TRUE(


I think here the estl api was way better. It also has different behaviour, checking the equality of the sizes. However it does look like etl has etl::eqal, why not use that here?

agreed. We could have used etl::equal here.

Agreed. Adjusted accordingly.

christian-schilling · 2025-02-10T13:33:32Z

libs/bsw/io/examples/MemoryQueueExample.cpp

+    frame.id = *reinterpret_cast<::etl::be_uint32_t*>(data.data());
+    data     = data.subspan(sizeof(etl::be_uint32_t));
+    ::etl::mem_copy(data.begin(), data.end(), frame.data.begin());
+    frame.data = frame.data.subspan(0, data.size());


.trim() as an inplace operation was introduced because some compilers used significantly more stack space when using .subslice(). This may or may not be an issue with the compilers being used in the future, but it certainly was quite significant in the past.

I have no knowledge about this issue, but subspan seems more clear than trim. I know estd also has subslice, but they seem to be doing the same thing (both etl::span::subspan and estd::slice::subslice).
Also the fact that trim may reset the size of the slice to zero seems to be strange in my view (even though it is documented).
But really not sure about this point.

christian-schilling · 2025-02-10T13:35:37Z

libs/bsw/io/include/io/ForwardingReader.h

@@ -84,7 +85,7 @@ inline ::estd::slice<uint8_t> ForwardingReader::peek() const
 inline void ForwardingReader::release()
 {
    // If _destinationData is an empty slice, copy will not copy any data.
-    (void)::estd::memory::copy(_destinationData, _sourceData);
+    (void)::etl::mem_copy(_sourceData.begin(), _sourceData.end(), _destinationData.begin());


estd::memory::copy is safer here as it also checks the destination length, which etl::mem_copy obviously can't.

if we want safe copies, etl also offers that in copy_s in etl/algorithm.h
Also etl::mem_copy shouldn't check the length since it is a low level copy (basically memcpy but with pointers of the same time), so in this case it makes sense.

Right.

When I previously contributed this addition to ETL:

ETLCPP/etl#1024
https://github.com/rolandreichweinbmw/etl/tree/span-advance-copy-reinterpret-as

... then I better also use it. :-)

Adjusting accordingly.

christian-schilling · 2025-02-10T13:55:39Z

libs/bsw/uds/mock/gmock/src/uds/connection/ResponseMock.cpp

-    auto const v = ::estd::be_uint32_t::make(data);
-    return appendData(v.bytes, 4) == 4;
+    auto const v = ::etl::be_uint32_t(data);
+    return appendData(static_cast<uint8_t const*>(v.data()), 4) == 4;


I think the estl api is more clear here.

I think the static_cast wasn't needed here. The method etl::unaligned_type_common::data() already returns a const uint8_t*.
Honestly since data is used on most containers to get a pointer to its raw buffer, I think data seems more aligned with the rest of the apis.
About the make method, seems to be a bit redundant.

Agree. I removed static_cast.

rolandreichweinbmw · 2025-02-10T17:09:36Z

I'm raising a few points where I see etl being a step back from estl to put them up for discussion. Apart from those we should think about how to treat etl and it's development. Do we want to upstream changes? Maintain patches? Keep it as is? Maintain an "add-on" library for missing pieces (like the ones I commented on).

Also I'm wondering if we really gain a lot by adopting etl. Just because we can does not mean we should.

During the process for this PR, I already contributed some additions to ETL to easen its use in OpenBSW, and the author is quite responsive:

https://github.com/ETLCPP/etl/issues?q=is%3Apr%20author%3Arolandreichweinbmw

Most of it he already merged, but unfortunately, they are not yet in the master branch or released yet due to the ETL development process.

However, I'm maintaining the changes relevant to OpenBSW in:

https://github.com/rolandreichweinbmw/etl/tree/openbsw-replace-estd - see also the individual feature branches.

I think for this purpose, we can maintain a fork or branches for changes not yet released in upstream ETL even though we should contribute all of it.

One point for ETL is that some of the features I would like to contribute to OpenBSW are already based on ETL. And ETL is being used in projects where I would like to merge OpenBSW. Apart from ETL being established in the industry and public since many years.

rolandreichweinbmw · 2025-02-28T17:19:06Z

Just updated the branch and PR with new ETL upstream update, based on new ETL version 20.40.0, but with yet unmerged changes upstream that we need for easy switchover of OpenBSW to ETL. (As before, copied into the openbsw branch "use-etl".)

New ETL branch with those additions for reference is: https://github.com/rolandreichweinbmw/etl/tree/openbsw-replace-estd

rolandreichweinbmw force-pushed the use-etl branch 3 times, most recently from b385542 to 80dff0c Compare February 8, 2025 23:08

christian-schilling reviewed Feb 10, 2025

View reviewed changes

rolandreichweinbmw force-pushed the use-etl branch from ac25775 to 812d4f8 Compare February 21, 2025 16:27

rolandreichweinbmw added 20 commits February 21, 2025 17:33

Switch from ESTD to ETL

f19c74d

Use new remove interface of etl::intrusive_forward_list

99906e6

Reintroduced singleton as singleton_base

8be9ba2

Reformat via treefmt

f906b5f

Adjust to newer ETL interfaces

9741d6a

Reformat with treefmt

039e776

Using ::etl::equal() for span comparison

be8a594

Use ::etl::copy() for spans

c91d946

Use etl::make_span()

124c6a9

Simplify usage of etl::unaligned_type interface

d075aa6

Use newer ETL interfaces

fbae275

Use new ETL interfaces

22dad8c

Reformat w/ treefmt

223eab9

Use new ETL interfaces

2c6868e

Use new ETL interfaces

2810084

Adjust to original .push_back() usage

7b1c9c1

Lambda setup

870c6a8

Fix format via treefmt

57f2d74

Temporarily revert back to estd function and closure

9dbdda1

Clean up remaining estd/functional.h

bde7dba

Adjust to new estd use

18c55db

rolandreichweinbmw mentioned this pull request Feb 21, 2025

Unit tests silently failing in CI #61

Open

rolandreichweinbmw force-pushed the use-etl branch from b3394ff to a95b467 Compare February 21, 2025 18:27

Updated ETL from upstream

cefb0c4

rolandreichweinbmw force-pushed the use-etl branch from a95b467 to cefb0c4 Compare February 28, 2025 16:56

Fixed format via treefmt

3da1792

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from ESTD to ETL #58

Switch from ESTD to ETL #58

rolandreichweinbmw commented Feb 7, 2025

marcmo commented Feb 8, 2025

rolandreichweinbmw commented Feb 8, 2025 •

edited

Loading

rolandreichweinbmw commented Feb 8, 2025 •

edited

Loading

rolandreichweinbmw commented Feb 9, 2025 •

edited

Loading

christian-schilling left a comment

christian-schilling Feb 10, 2025

rolandreichweinbmw Feb 10, 2025 •

edited

Loading

filipe-cuim Feb 10, 2025

christian-schilling Feb 10, 2025

filipe-cuim Feb 10, 2025

rolandreichweinbmw Feb 10, 2025

christian-schilling Feb 10, 2025

filipe-cuim Feb 10, 2025 •

edited

Loading

rolandreichweinbmw Feb 10, 2025

christian-schilling Feb 10, 2025

filipe-cuim Feb 10, 2025

christian-schilling Feb 10, 2025

filipe-cuim Feb 10, 2025

rolandreichweinbmw Feb 10, 2025

christian-schilling Feb 10, 2025

filipe-cuim Feb 10, 2025

christian-schilling Feb 10, 2025

filipe-cuim Feb 10, 2025

rolandreichweinbmw Feb 10, 2025

christian-schilling Feb 10, 2025

filipe-cuim Feb 10, 2025

rolandreichweinbmw Feb 10, 2025

rolandreichweinbmw commented Feb 10, 2025 •

edited

Loading

rolandreichweinbmw commented Feb 28, 2025

Switch from ESTD to ETL #58

Are you sure you want to change the base?

Switch from ESTD to ETL #58

Conversation

rolandreichweinbmw commented Feb 7, 2025

marcmo commented Feb 8, 2025

rolandreichweinbmw commented Feb 8, 2025 • edited Loading

rolandreichweinbmw commented Feb 8, 2025 • edited Loading

rolandreichweinbmw commented Feb 9, 2025 • edited Loading

christian-schilling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolandreichweinbmw Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipe-cuim Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolandreichweinbmw commented Feb 10, 2025 • edited Loading

rolandreichweinbmw commented Feb 28, 2025

rolandreichweinbmw commented Feb 8, 2025 •

edited

Loading

rolandreichweinbmw commented Feb 8, 2025 •

edited

Loading

rolandreichweinbmw commented Feb 9, 2025 •

edited

Loading

rolandreichweinbmw Feb 10, 2025 •

edited

Loading

filipe-cuim Feb 10, 2025 •

edited

Loading

rolandreichweinbmw commented Feb 10, 2025 •

edited

Loading